Ignore numeric-looking strings with huge values #18
Conversation
shadowspawn
changed the title
Codecov Report
@@ Coverage Diff @@
## main #18 +/- ##
=======================================
Coverage 98.75% 98.76%
=======================================
Files 1 1
Lines 161 162 +1
Branches 71 72 +1
=======================================
+ Hits 159 160 +1
Misses 2 2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
|
| @@ -12,6 +12,7 @@ function hasKey(obj, keys) { | |||
|
|
|||
| function isNumber(x) { | |||
| if (typeof x === 'number') { return true; } | |||
| if (!Number.isSafeInteger(Math.floor(x))) { return false; } | |||
There was a problem hiding this comment.
isSafeInteger isn’t available in all the engines we support.
There was a problem hiding this comment.
Yes. I looked up when isSafeInteger was introduced but then forgot to check the oldest supported version for Minimist, or lack of an explicit minimum. Realised when I saw the test fail.
The PR isn't a no-brainer so I'll only try and make it work with older nodes if otherwise of interest?
There was a problem hiding this comment.
| if (!Number.isSafeInteger(Math.floor(x))) { return false; } | |
| if (x >= Math.pow(2, 53)) { return false; } |
i think this would do it?
My concern is whether this bugfix could arguably be considered a breaking change, in which case it wouldn't likely be worth it.
There was a problem hiding this comment.
Yes, being conservative it is arguably a breaking change, and it isn't a full solution.
|
I wanted to look into it, and have! Have some clues for future if needed. Not compelling enough for now. |
|
For interest, I found a copy of the original reported Minimist bug: meszaros-lajos-gyorgy/minimist-lite#2 |
Actually, can specify positional are "string" by specifying |
|
I noticed this is touched on in the Open Group Base Specifications for Utility Conventions
So there is some precedent for limiting the numbers, if we choose to in the future. |
An issue people encounter using minimist (as reported on old repository) is that false-positive numeric-looking strings are unexpectedly converted to numbers with data loss, whether losing precision or being converted to
infinity. For example, values for twitter or YouTube ids.The README says:
The suggested author work-around of explicitly setting the option parsing does avoid the automatic number conversion for options, but this approach is not available for positional arguments.
A possible improvement is to follow the lead of yargs and not attempt to coerce numeric-looking strings which are too big to reasonably represent: yargs/yargs-parser#110
(This only addresses one issue with automatic numeric conversion, and I don't mind if we leave it out and keep it simple! I had seen the issue reported and wanted to look into the problem, and how the code had evolved in Yargs which I knew had added some extra checks.)